Repository navigation
fix: generate signed byte types for VT_I1 - #962
haoxuanjng-lang wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #962 +/- ##
==========================================
+ Coverage 89.34% 89.46% +0.12%
==========================================
Files 142 143 +1
Lines 13856 14045 +189
==========================================
+ Hits 12380 12566 +186
- Misses 1476 1479 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
junkmd
left a comment
junkmd
left a comment
There was a problem hiding this comment.
Thank you for your continued contributions!
This is clearly an inconsistency, and it is normally not the correct behavior for software. However, I could not find any earlier bug reports about this specific behavior. This makes me wonder whether users have come to expect the current behavior over time, or whether no one has ever reached this code path.
So, let's move forward step by step, checking both the impact of this change and backward compatibility.
As before, let's start with the test structure.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
|
Added the reverse-direction probe and expanded the real native-call matrix. The separate component/interface/test module are now named The output probe writes one character through a plain Observed with freshly generated wrappers against latest-main production versus the proposed mapping:
This establishes the opposite direction's accepted-buffer change independently of SAFEARRAYs. Explicit LPSTR and unsigned scalar tables still pass both versions. I retained the two legacy input/output assertions as expected failures and added documentation for affected generated types, saved/handwritten interfaces, replacement arguments, signed outputs and explicit string handling. Full local unittest: 474 run / 448 passed / 24 skipped / 2 expected compatibility failures. Focused suites: 70 / 59 / 9 / 2; without NumPy/pywin32: 51 / 41 / 8 / 2. Native build, full Ruff CI scope and strict Sphinx HTML build pass. Existing full docs doctest cannot get past its optional Excel type-library setup locally ( The mapping itself is unchanged in this follow-up and the PR remains draft. These results and the documentation describe the compatibility cost; they do not resolve it or decide whether a more detailed mapping should be used. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
junkmd
left a comment
junkmd
left a comment
There was a problem hiding this comment.
This PR is logically consistent, regardless of compatibility, and the tests have become clearer with each review. However, I could not explain why it still felt difficult to understand.
I have just realized that the tests do not yet give us a clear record of this compatibility change.
At the moment, the tests only cover the new mapping. It was hard to compare how the old mapping worked, and what happens when we use the old calling style with the new mapping.
I came up with the fixtures to make this easier to understand.
This mapping seems to have survived unchanged for about 20 years, at least as far back as our version history goes. Changing something that old feels A LITTLE scary!
Please bear with me a little longer, at least until I can understand this change well enough myself.
Thank you!
This comment was marked as outdated.
This comment was marked as outdated.
junkmd
left a comment
junkmd
left a comment
There was a problem hiding this comment.
I think it has become much easier to understand.
I'll just mention one thing that caught my attention when I briefly glanced at the code.
| @unittest.expectedFailure | ||
| def test_char_pointer_bytes_input(self): | ||
| self.assertEqual(self.server.ReadCharPointer(b"h"), 104) | ||
|
|
||
| @unittest.expectedFailure | ||
| def test_character_bytes_output_buffer(self): | ||
| result = create_string_buffer(2) | ||
| self.server._IByteCompatibilityTest__com_GetCharacterBytes(104, result) | ||
| self.assertEqual(result.value, b"h") |
There was a problem hiding this comment.
Since expectedFailure hides the reason for the failure, it might be better to use assertRaises instead.
If an assertion fails, please use assertNotEqual for the expected value in the legacy mapping and assertEqual for the expected value in the new mapping, and add a clarifying comment.
|
|
||
|
|
||
| @unittest.skipIf(IMPORT_SERVER_FAILED, "Requires the C++ out-of-process COM server.") | ||
| class ByteCompatibilityTest(unittest.TestCase): |
There was a problem hiding this comment.
I would like you to structure the tests like the example below: please divide the test cases by method of the interface, and separate them into a happy path and unhappy paths (broken down by each error type).
class TestEchoInt8(unittest.TestCase):
def test_takes_and_returns_as_intended(self):
"""Happy path: Combinations of input values and expected results"""
def test_raises_type_error(self):
"""Unhappy path (TypeError): Coverage of errors"""
def test_raises_argument_error(self):
"""Unhappy path (ArgumentError): Coverage of errors"""
class TestEchoUInt8(unittest.TestCase):
def test_takes_and_returns_as_intended(self):
"""Happy path: Combinations of input values and expected results"""
def test_raises_type_error(self):
"""Unhappy path (TypeError): Coverage of errors"""
def test_raises_argument_error(self):
"""Unhappy path (ArgumentError): Coverage of errors"""
...| def test_legacy_int8_implicit_output(self): | ||
| # STRING cannot allocate a scalar output. Keep that legacy behavior | ||
| # visible rather than silently adapting it to the proposed result type. | ||
| for value in (b"\x80", b"\xf9", b"\xff", b"\0", b"\x01", b"\x7f"): | ||
| with self.subTest(mapping="legacy", value=value): | ||
| with self.assertRaises(TypeError): | ||
| self.legacy.EchoInt8(value) | ||
|
|
||
| def test_legacy_int8_explicit_output(self): | ||
| for value in (b"\x80", b"\xf9", b"\xff", b"\0", b"\x01", b"\x7f"): | ||
| with self.subTest(mapping="legacy", value=value): | ||
| result = create_string_buffer(2) | ||
| self.legacy._IByteCompatibilityTest__com_EchoInt8(value, result) | ||
| self.assertEqual(result.raw[:1], value) |
There was a problem hiding this comment.
Performing buffer creation and result slicing—which explicit_output does—inside the tests might be reducing readability.
Please split the create_legacy_itf factory into two versions: one that overrides the EchoInt8 method to encapsulate buffer creation and result slicing, and one that does not. And please give each of those factories a good, distinguishable name. Then, please place them into the happy path and unhappy path subtests.
Please move comments like # STRING cannot allocate a scalar output. to the section where the method is overridden and encapsulated within the factory.
There was a problem hiding this comment.
I noticed this test module name is too general.
The names of the test double and the test case are fine as they are, but something like test_compatibility_tlbparser_VT_I1_assignment might be better for the test module name.
If we use this kind of name, we can clearly see which issue it is about and what theme it has. If we only say "byte compatibility", people might misunderstand it in many different ways.
Generating
VT_I1asc_charchanges native signed values such as -7 to 249 in tuples and produces byte-string arrays or reshape errors with NumPy. This PR proposessigned char/c_byteforVT_I1, while explicitVT_LPSTRremains a narrow string. Context: #956 and #935. The intermittent UIA exception discussed in the latter remains outside this change.Draft: the plain character-pointer compatibility decision remains pending. Old bytes input and writable character-buffer output still raise
ArgumentErroron the proposed generated interface. The two regression assertions remain explicit expected failures.The separate native
CoByteCompatibilityTestcomponent supplies real out-of-process COM calls. Following the review,test_cpp_byte_compatibility.pynow defines both legacy and proposedCOMMETHODlists in the same module. Interface factories expose the same method names, with automatic, pointer, byref and array output helpers. The tests compare accepted input types and literal return values across these interfaces. A unittest assertion checks that freshly generated definitions match the proposed list; a separate old-production validation verifies the legacy list exactly.The comparison records both directions: legacy
ReadCharPointeraccepts bytes and rejects numeric pointer/reference/array arguments; all four proposed interfaces do the reverse. LegacyGetCharacterBytesreturns bytes from caller-owned character storage, while the proposed interfaces return signed integers. The legacy scalar's automatic STRING output raisesTypeErroreven with a one-byte bytes input; explicit character storage preserves the original byte. This behavior is recorded separately from the proposed signed scalar results. Explicit LPSTR and unsigned values are checked across all five interfaces. LPSTR helpers free the allocated address infinally; plain character output reserves a local terminator without assuming native NUL termination.Independent 1-D/2-D SAFEARRAY value/shape/dtype/copy checks and the migration page remain. This follow-up changes only the Python compatibility test module; production mapping and native source are unchanged from the previous revision, based on main
91408c25247bec90b09db0bbc7717f47438cb06f.Observed local validation on Windows x64, Python 3.13.5, NumPy 2.1.3 and pywin32 308:
python -m unittest discover -v -s comtypes/test -t comtypes/test: 484 run / 458 passed / 24 skipped / 2 expected compatibility failures.git diff --checkpasses.The unchanged native fixture was rebuilt and validated in the preceding revision. Documentation and the optional-dependency-free environment were not rerun for this test-only revision; earlier results are separate evidence. The latest published head's upstream CI is recorded after push, with pending checks kept distinct from local results. The PR remains draft pending the compatibility strategy.
AI assistance: OpenAI Codex helped investigate, implement and validate this contribution.